Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

planner, executor: fix haven't track the memory usage of PointGet/BatchPointGet #21230

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

zhaoxugang
Copy link
Contributor

@zhaoxugang zhaoxugang commented Nov 23, 2020

fix haven't track the memroy usage of PointGet/BatchPointGet

What problem does this PR solve?

Issue Number: close #21653

Problem Summary:

What is changed and how it works?

What's Changed:
enable memory tracker in point_get/batch_point_get

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Release note

  • planner, executor: fix haven't track the memroy usage of PointGet/BatchPointGet

…works and enable memory tracker in point_get/batch_point_get
@zhaoxugang zhaoxugang requested review from a team as code owners November 23, 2020 15:47
@zhaoxugang zhaoxugang requested review from wshwsh12 and winoros and removed request for a team November 23, 2020 15:47
@ti-srebot ti-srebot added the first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. label Nov 23, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Nov 23, 2020

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

  • *: what's changed

@sre-bot
Copy link
Contributor

sre-bot commented Nov 23, 2020

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

  • *: what's changed

@github-actions github-actions bot added sig/execution SIG execution sig/planner SIG: Planner labels Nov 23, 2020
@zhaoxugang zhaoxugang changed the title fix statement-level optimize hint will be ignored when tryFastPlan fix statement-level optimize hint will be ignored when tryFastPlan works Nov 23, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Nov 23, 2020

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

  • *: what's changed

@zhaoxugang zhaoxugang changed the title fix statement-level optimize hint will be ignored when tryFastPlan works fix statement-level optimize hint invalid and memory tracker when tryFastPlan works Nov 23, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Nov 23, 2020

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

  • *: what's changed

@ghost ghost changed the title fix statement-level optimize hint invalid and memory tracker when tryFastPlan works planner, executor: fix statement-level optimize hint invalid and memory tracker when tryFastPlan works Nov 23, 2020
@ichn-hu ichn-hu mentioned this pull request Nov 24, 2020
@zhaoxugang
Copy link
Contributor Author

PTAL @winoros @wshwsh12

@zhaoxugang
Copy link
Contributor Author

/run-all-tests

Copy link
Contributor

@wshwsh12 wshwsh12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we haven't track the memroy usage of PointGet/BatchPointGet. But I think we should use another issue/pr to track it, not this. One pr should only do one thing.

Rest LGTM @zhaoxugang

session/session_test.go Show resolved Hide resolved
@wshwsh12
Copy link
Contributor

/reward 300

@ti-challenge-bot
Copy link

This PR's linked issue is not picked.

@zhaoxugang zhaoxugang changed the title planner, executor: fix statement-level optimize hint invalid and memory tracker when tryFastPlan works planner, executor: fix haven't track the memroy usage of PointGet/BatchPointGet Dec 11, 2020
@ti-chi-bot
Copy link
Member

Merge canceled because a new commit is pushed.

@ti-chi-bot ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Feb 22, 2021
@ti-chi-bot
Copy link
Member

@zhaoxugang: Your PR has out-of-dated, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@ti-chi-bot ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 22, 2021
@CLAassistant
Copy link

CLAassistant commented Feb 22, 2021

CLA assistant check
All committers have signed the CLA.

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 7, 2021
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 7, 2021
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2021
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhaoxugang thanks for your contribution, please resolve conflicts and address the comments.

@@ -3161,6 +3161,32 @@ func (s *testSessionSuite2) TestStmtHints(c *C) {
c.Assert(tk.Se.GetSessionVars().GetReplicaRead(), Equals, kv.ReplicaReadFollower)
}

// Test memory track in POINT_GET/BATCH_POINT_GET for issue 21653
func (s *testSessionSuite2) TestPointGetMemoryTracking(c *C) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Could you directly validate the tracked memory usage?
  2. Please also add tests for clustered index whose type is not an integer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't directly validate the memory usage,do you have some advices to do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, what is the difference between integer and others, why need add tests for clustered index whose type is not an integer.

@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 28, 2021
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2021
@hawkingrei
Copy link
Member

hawkingrei commented Jan 23, 2022

@zhaoxugang Thank you for your contribution. Do you have time to solve the conflicts? If there is no reply within a week, I will close the PR and take over the job.

# Conflicts:
#	executor/batch_point_get.go
#	executor/point_get.go
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2022
@zhaoxugang
Copy link
Contributor Author

/run-check_dev

@sre-bot
Copy link
Contributor

sre-bot commented Feb 12, 2022

@zhaoxugang
Copy link
Contributor Author

@zhaoxugang Thank you for your contribution. Do you have time to solve the conflicts? If there is no reply within a week, I will close the PR and take over the job.

done

@hawkingrei
Copy link
Member

@wshwsh12 PTAL

@ti-chi-bot
Copy link
Member

@zhaoxugang: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2022
@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Attention: Patch coverage is 83.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 63.8708%. Comparing base (2ae8529) to head (47e7c0a).
Report is 12277 commits behind head on master.

Additional details and impacted files
@@                Coverage Diff                @@
##             master     #21230         +/-   ##
=================================================
- Coverage   77.1798%   63.8708%   -13.3090%     
=================================================
  Files           588        773        +185     
  Lines        148465     257205     +108740     
=================================================
+ Hits         114585     164279      +49694     
- Misses        23589      79824      +56235     
- Partials      10291      13102       +2811     

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/execution SIG execution sig/planner SIG: Planner sig/sql-infra SIG: SQL Infra size/M Denotes a PR that changes 30-99 lines, ignoring generated files. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

haven't track the memory usage of PointGet/BatchPointGet